Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @rodrigo-pino)

crates/blockifier/src/execution/native/syscall_handler.rs line 110 at r2 (raw file):

    pub fn substract_syscall_gas_cost(
        &mut self,
        remaining_gas: &mut u128,

https://reviewable.io/reviews/starkware-libs/sequencer/621#-O7AR9rr3iLF7ozeCslU
see discussion above

Suggestion:

remaining_gas: &mut u128,
remaining_gas: &mut u64,

crates/blockifier/src/execution/native/utils.rs line 13 at r2 (raw file):

pub fn encode_str_as_felts(msg: &str) -> Vec<Felt> {
    const CHUNK_SIZE: usize = 32;

Can you please explain? Why 32?

Code quote:

const CHUNK_SIZE: usize = 32;

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 5 unresolved discussions (waiting on @meship-starkware and @rodrigo-pino)

crates/blockifier/src/execution/native/syscall_handler.rs line 44 at r2 (raw file):

    pub read_values: Vec<Felt>,
    pub accessed_keys: HashSet<StorageKey, RandomState>,
}

Writing here for the record additional field of the cairo1 vm hint proccessor:
read_only_segments
secp256k\r1_hint_processor
sha256_segment_end_ptr
hints
execution_info_ptr

crates/blockifier/src/execution/native/syscall_handler.rs line 87 at r2 (raw file):

        self.update_remaining_gas(remaining_gas, &call_info);

        let retdata = call_info.execution.retdata.clone();

Suggestion:

        let retdata = call_info.execution.retdata.clone();

        if call_info.execution.failed {
            // In VM it's wrapped into `SyscallExecutionError::SyscallError`.
            return Err(retdata.o.clone());
        }

        self.update_remaining_gas(remaining_gas, &call_info);

crates/blockifier/src/execution/native/syscall_handler.rs line 103 at r2 (raw file):

        // Change the remaining gas value.
        *remaining_gas = u128::from(remaining_gas_u64);
    }

Please move this method outside the Imp

Suggestion:

    pub fn update_remaining_gas(remaining_gas: &mut u128, call_info: &CallInfo) {
        let mut remaining_gas_u64 = u64::try_from(*remaining_gas).expect("Failed to convert gas to u64.");

        update_remaining_gas(&mut remaining_gas_u64, call_info);

        *remaining_gas = u128::from(remaining_gas_u64);
    }

Reviewable status: 1 of 4 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @rodrigo-pino)

crates/blockifier/src/execution/native/utils.rs line 12 at r2 (raw file):

}

pub fn encode_str_as_felts(msg: &str) -> Vec<Felt> {

See discussion
https://reviewable.io/reviews/starkware-libs/sequencer/621#-O5wl5XLBruyQJwstLkp

Reviewed 1 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @noaov1 and @rodrigo-pino)

Reviewable status: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @noaov1)

crates/blockifier/src/execution/native/syscall_handler.rs line 87 at r2 (raw file):

        self.update_remaining_gas(remaining_gas, &call_info);

        let retdata = call_info.execution.retdata.clone();

👍

crates/blockifier/src/execution/native/syscall_handler.rs line 103 at r2 (raw file):

I don't know why this were public execute_inner_call, update_remaining_gas and substract_syscall_gas_cost. I've reverted them back to private and used #[allow(dead_code)] for the time being.

I can also removed them but almost all syscalls will be using them in the future which means a bit slowness when starting those.

Is in the impl because it was meant as an internal part of NativeSyscallHandler private to everyone else. If you still think is best out I'll move it out

crates/blockifier/src/execution/native/syscall_handler.rs line 110 at r2 (raw file):

Native treats gas as a u128 hence we have to build our wrappers around it to communicate with blockifiers u64. We can ask for a u64 change on their part and re-update this part.

crates/blockifier/src/execution/native/utils.rs line 12 at r2 (raw file):

This functions are required to handle user created errors (either panics! or Err results) or inner-call error propagation.

If we get an error message which fits in a Felt then it behavesthe same as the VM.

If the error is bigger, meaning it spans several Felt, we take the bytes of each felt, join them and decode them as utf8 if possible. If not utf8-decodable, then we just try to decode individual Felts.

This method is only used after Native returned control and there was an error. The error returned by Native won't be in String form but in a Vec<Felt> hence the need to decode it.

crates/blockifier/src/execution/native/utils.rs line 13 at r2 (raw file):

We are converting a string back into a Felt. We then created small arrays of 32 bytes, fill the first 31 bytes with the string encoding information and make them Felts.

Reviewed 3 of 17 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: 4 of 17 files reviewed, 7 unresolved discussions (waiting on @meship-starkware and @rodrigo-pino)

.github/workflows/blockifier_ci.yml line 44 at r7 (raw file):

jobs:
  featureless-build:
    runs-on: ubuntu-22.04

Why was this change needed?
@alon-dotan-starkware

Code quote:

    runs-on: ubuntu-22.04

.github/workflows/main.yml line 22 at r7 (raw file):

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }}

Why was this removed?

Code quote:

# On PR events, cancel existing CI runs on this same PR for this workflow.
concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }}

crates/blockifier/src/execution/native/syscall_handler.rs line 103 at r2 (raw file):

RE the private: great!
I thought that as it does not use self it is better to move it out of the impl block.

Reviewed 1 of 4 files at r9, 1 of 1 files at r11, 1 of 1 files at r13, 1 of 1 files at r14, 1 of 1 files at r16, 10 of 10 files at r17, all commit messages.
Reviewable status: 15 of 17 files reviewed, 8 unresolved discussions (waiting on @meship-starkware and @rodrigo-pino)

crates/blockifier/src/execution/native/syscall_handler.rs line 112 at r17 (raw file):

                    .expect("Failed to parse OUT_OF_GAS_ERROR hex string"),
            ]);
        }

Where do we set the failure flag to 1 in this case? (and in any case of an error returned from a syscall)

Code quote:

            return Err(vec![
                Felt::from_hex(OUT_OF_GAS_ERROR)
                    .expect("Failed to parse OUT_OF_GAS_ERROR hex string"),
            ]);
        }

crates/blockifier/src/execution/native/syscall_handler.rs line 320 at r17 (raw file):

fn native_update_remaining_gas(remaining_gas: &mut u128, call_info: &CallInfo) {
    // Create a new variable with converted type.
    let mut remaining_gas_u64 = u64::try_from(*remaining_gas).unwrap();

Suggestion:

let mut remaining_gas_u64 = u64::try_from(*remaining_gas).expect("Failed to convert gas to u64.");

crates/blockifier/src/execution/native/utils.rs line 13 at r2 (raw file):

Why 32 exactly?

Reviewable status: 14 of 17 files reviewed, 8 unresolved discussions (waiting on @meship-starkware and @noaov1)

crates/blockifier/src/execution/native/syscall_handler.rs line 103 at r2 (raw file):

yes, it makes sense!

crates/blockifier/src/execution/native/syscall_handler.rs line 112 at r17 (raw file):

When Native invokes a syscall and passes control to us, it expects a a result a SyscallResult<OkType, ErrType>. If we return an Err in the syscall, in this specific context by calling this function, it will propagate back to Native. The Native Contract should then propagate the error back and return it as well as setting the failure flag to 1

crates/blockifier/src/execution/native/syscall_handler.rs line 320 at r17 (raw file):

fn native_update_remaining_gas(remaining_gas: &mut u128, call_info: &CallInfo) {
    // Create a new variable with converted type.
    let mut remaining_gas_u64 = u64::try_from(*remaining_gas).unwrap();

👍

crates/blockifier/src/execution/native/utils.rs line 13 at r2 (raw file):

Ok, so the issue to solve is we have a long string we want to encode as Felts. We know how much info fits in a Felt (31 Bytes).

The function splits the string in chunks of 31 Bytes, copies them to a static array of size 32 Bytes which is then used to create the new felt with Felt::from_bytes_be(...). This function requires the static array size to be 32 (maybe this is what you were asking)

During the code we are either dealing with CHUNK_SIZE - 1 or CHUNK_SIZE.

.github/workflows/main.yml line 22 at r7 (raw file):

Unintentional deletion.

Reviewed 2 of 4 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 1 files at r13, 1 of 1 files at r14, 1 of 1 files at r16, 10 of 10 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @noaov1 and @rodrigo-pino)

Reviewed 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

crates/blockifier/src/execution/native/utils.rs line 13 at r2 (raw file):

Got it. Thanks!
Why not do it as we encode the constants error (e.g. OUT_OF_GAS_ERROR):

Code snippet:

let bytes = msg.as_bytes();
let error_felt = BigUint::from_bytes_be(bytes)
let error_hex = format!("0x{:x}", error_felt)
 Felt::from_hex(error_hex)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rodrigo-pino)

Reviewable status: 16 of 18 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

crates/blockifier/src/execution/native/utils.rs line 13 at r2 (raw file):

In this case we are just handling case where msg cannot have more than 31 bytes of information, otherwise it might not fit in a felt.

I remember we took that decision because there are errors given by the blockifier which exceed the 31 byte size, and there was no way of propagating all the information back through Native the same way you do in the VM with the Blockifier.

For example, the following error, there is no way to propagate back with Native without encoding it as Felts: